Skip to content

[ruby] fix: build_from_hash drops every attribute inherited from an allOf parent - #24892

Merged
wing328 merged 8 commits into
OpenAPITools:masterfrom
wiebren:fix/ruby-build-from-hash-inherited-attributes
Sep 25, 2026
Merged

wing328 merged 8 commits into
OpenAPITools:masterfrom
wiebren:fix/ruby-build-from-hash-inherited-attributes

Conversation

@wiebren

@wiebren wiebren commented Sep 7, 2026 •

Copy link
Copy Markdown
Contributor

A discriminator-based allOf child generates as a subclass (class Cat < Animal), but its build_from_hash maps only the child's own attributes:

  • a required inherited attribute makes deserialization raise;
  • an optional inherited attribute silently comes back nil.
Petstore::Cat.build_from_hash({ 'className' => 'Cat', 'color' => 'black', 'declawed' => true })
# master:   ArgumentError: class_name cannot be nil
# this PR:  #<Petstore::Cat class_name="Cat", color="black", declawed=true>

to_hash has the mirror defect: a round-tripped child loses its parent's attributes on the way out. Reproduced on current master.

Why super(attributes) didn't help

openapi_types and attribute_map dispatch on the child class even in the parent's frame, so the super call only built a second instance from the child's own attributes and discarded it. It is removed.

Fix

partial_model_generic.mustache adds two class methods that use the same idiom as the existing acceptable_attribute_map (the nearest declaration wins):

def self.acceptable_openapi_types      # without a generated parent: openapi_types
  superclass.acceptable_openapi_types.merge(openapi_types)
end
def self.acceptable_openapi_nullable   # without a generated parent: openapi_nullable
  (superclass.acceptable_openapi_nullable - attribute_map.keys) | openapi_nullable
end

The delegation to superclass is gated on {{#parentModel}} (a generated parent), not {{#parent}}. This covers these helpers, acceptable_attribute_map and the initializer's super(attributes). An alias model such as ArrayAlias < Array has a parent but no generated parent, so every model gets a base case; array_alias.rb in the alias-as-model sample is the only file this gate changes.

In base_object.mustache, build_from_hash iterates acceptable_openapi_types and looks keys up in acceptable_attribute_map, the map the generated initializer validates against, so new only receives attributes it accepts. to_hash iterates acceptable_attribute_map and checks acceptable_openapi_nullable. A consumer subclass that narrows attribute_map behaves as on master.

Tests

  • RubyClientCodegenTest#testBuildFromHashMapsInheritedAttributes generates Lizard < Reptile < Pet from 3_0/allOf_composition_discriminator.yaml and asserts the merge methods, their use in build_from_hash/to_hash, and that the discarded super(attributes) is gone. Fails without the fix.
  • RubyClientCodegenTest#testAliasModelDoesNotDelegateToItsBuiltInSuperclass generates 3_0/features/generate-alias-as-model.yaml and asserts that ArrayAlias < Array has base-case helpers and no superclass/super(attributes) delegation.
  • spec/custom/base_object_spec.rb in the typhoeus, faraday and httpx petstore samples: a Cat round trip that keeps className/color, and a regression guard for a subclass that narrows attribute_map.

PR checklist

  • Read the contribution guidelines.
  • Built the project and updated samples (./bin/generate-samples.sh ./bin/configs/ruby*.yaml). Every ruby-client model changes, so the sample diff is broad but mechanical. ruby-nextgen is unaffected.
  • Technical committee: @cliffano @zlx @autopp

Summary by cubic

Fixes build_from_hash and to_hash in the Ruby client dropping attributes a model inherits from its allOf parents. A requirement bug where the attribute is:

  • A discriminator has its parent's attributes (fixed and a parent's approach)
  • child < parent raises build_class:
Petstore::Cat.build_from_hash({ 'className' => 'Cat', 'color' => 'black', 'declawed' => true })
# master:   ArgumentError: class_name cannot be nil                     # this PR:  #<Petstore::Cat class_name="Cat", color="black", declawed=true>

This approach adds two class methods with the same parent-gated idiom as the existing acceptable_attribute_map (stable attribute for classes whose superclass is parentModel); the children).---

Written for commit 3abb381. Summary will update on new commits.

Review in cubic


Generated with Claude Code

wiebren and others added 2 commits September 7, 2026 16:53
A discriminator allOf child generates as a real subclass, but
build_from_hash iterated only openapi_types/attribute_map - class methods
that dispatch on the child class in every frame of the ancestry, so a
parent's attributes were never mapped from the wire hash: a required
inherited attribute made deserialization raise ArgumentError, an optional
one came back silently nil. The pre-existing super(attributes) call could
not help: its result was discarded, and it iterated the child's own types
anyway.

Walk the ancestry merging each ancestor's openapi_types and attribute_map
(the child wins on a name clash) and drop the discarded-result super
call. RubyClientCodegenTest asserts the walk on the generated two-level
Lizard < Reptile < Pet fixture (fails without the template change), and
the three petstore custom base_object specs now deserialize a Cat that
carries its parent's attributes.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GcwZ1arjLZNpetHz2a3TJz
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GcwZ1arjLZNpetHz2a3TJz

@cubic-dev-ai cubic-dev-ai Bot left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 220 files

Note: This PR contains a large number of files. cubic selects up to 200 of the highest-priority eligible files for this review, so some files may not have been reviewed.

Re-trigger cubic

Comment thread samples/client/petstore/ruby-faraday/lib/petstore/models/cat.rb Outdated
Comment thread samples/client/petstore/ruby-autoload/lib/petstore/models/cat.rb Outdated
Comment thread samples/client/petstore/ruby/lib/petstore/models/cat.rb Outdated
Comment thread samples/client/petstore/ruby-faraday/lib/petstore/models/dog.rb Outdated
Comment thread modules/openapi-generator/src/main/resources/ruby-client/base_object.mustache Outdated
wiebren and others added 2 commits September 7, 2026 21:27
The serialization direction has the identical defect: attribute_map and
openapi_nullable dispatch on the child class in every ancestor frame, so
the inherited super chain only repeated the child's own attributes and a
round-tripped allOf child lost its parent's fields again on the way out.
Walk the ancestry the same way build_from_hash now does; for a model
without a generated parent the walk does not run and the output is
unchanged.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GcwZ1arjLZNpetHz2a3TJz
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GcwZ1arjLZNpetHz2a3TJz
@wiebren

wiebren commented Sep 7, 2026

Copy link
Copy Markdown
Contributor Author

cubic's first remark was a real catch, now fixed: to_hash had the identical defect in the serialization direction - attribute_map/openapi_nullable dispatch on the child class in every ancestor frame, so the inherited super chain only repeated the child's own attributes and a round-tripped allOf child lost its parent's fields again on the way out. Verified before the fix: Cat.build_from_hash({className, color, declawed}).to_hash returned {declawed: true}. to_hash now walks the ancestry the same way build_from_hash does; for a model without a generated parent the walk does not run and the output hash is unchanged (same key order included). The petstore custom specs gain the round-trip case, and RubyClientCodegenTest asserts the walk.

On the other remark (a consumer subclass with its own attribute_map and a strict initializer now receiving parent keys): the generated initializers accept exactly the keys the walk maps - acceptable_attribute_map is the same ancestry merge - so every generated chain is consistent. A hand-written subclass that rejects its superclass's attributes in initialize was already producing objects with those fields silently dropped; the existing ArrayMapObject fixture (exactly such a subclass) still passes because its inputs carry none of its parent's keys. I'd rather surface that mismatch than silently drop wire data, but happy to add a filter if a maintainer prefers.

@cubic-dev-ai cubic-dev-ai Bot left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 219 files (changes from recent commits).

Note: This PR contains a large number of files. cubic selects up to 200 of the highest-priority eligible files for this review, so some files may not have been reviewed.

Re-trigger cubic

Comment thread samples/client/petstore/ruby-faraday/lib/petstore/models/pet.rb Outdated
Comment thread samples/client/petstore/ruby-autoload/lib/petstore/models/special_model_name.rb Outdated
Comment thread samples/client/petstore/ruby/lib/petstore/models/foo_get_default_response.rb Outdated
Comment thread samples/client/petstore/ruby-faraday/lib/petstore/models/nullable_class.rb Outdated
Comment thread samples/client/petstore/ruby/lib/petstore/models/zebra.rb Outdated
Comment thread samples/client/petstore/ruby-httpx/lib/petstore/models/list.rb Outdated
Comment thread samples/client/petstore/ruby-autoload/lib/petstore/models/client.rb Outdated
…nd Set loads

Two findings from review: the merged-nullable union let a parent's
nullable flag survive a child redeclaring the attribute as non-nullable -
an ancestor's entry now only applies to attributes no nearer class
declares - and the walk evaluates openapi_nullable eagerly for every
to_hash call, so the generated models require 'set' explicitly instead of
leaning on the Set builtin autoload rubies before 3.2 do not have.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GcwZ1arjLZNpetHz2a3TJz
@wiebren

wiebren commented Sep 8, 2026

Copy link
Copy Markdown
Contributor Author

Both of cubic's round-2 findings were real and are fixed:

  • Nullable precedence: the merged-nullable union let an ancestor's nullable flag survive a child redeclaring the attribute as non-nullable. The walk now adds an ancestor's nullable entries only for attributes no nearer class declares - the same nearest-declaration-wins rule openapi_types/attribute_map already follow. The custom specs gain a case pinning both directions (the ancestor's own instance still serializes its explicit nil; the redeclaring child omits it).
  • Set availability: the walk evaluates openapi_nullable on every to_hash call, where the old code only touched it for a nil value - so on rubies before 3.2 (no Set builtin autoload) an SDK loaded without something else requiring set would raise NameError. The generated models now require 'set' themselves.

All ruby samples regenerated; RubyClientCodegenTest pins the corrected walk and the require.

@wing328

wing328 commented Sep 8, 2026

Copy link
Copy Markdown
Member

@wiebren thanks for all the PRs to improve this project.

When you've time, can you please PM me via Slack for a quick discussion on these PRs?

https://join.slack.com/t/openapi-generator/shared_invite/zt-36ucx4ybl-jYrN6euoYn6zxXNZdldoZA

@cubic-dev-ai cubic-dev-ai Bot left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 262 files (changes from recent commits).

Note: This PR contains a large number of files. cubic selects up to 200 of the highest-priority eligible files for this review, so some files may not have been reviewed.
Tip: Review your code locally with the cubic CLI to iterate faster.

Re-trigger cubic

Comment thread samples/client/echo_api/ruby-httpx/lib/openapi_client/models/string_enum_ref.rb Outdated
wiebren and others added 2 commits September 21, 2026 15:20
…oad Set only where used

build_from_hash and to_hash now take their attribute map from
acceptable_attribute_map, the same map the generated initializer
validates against. For a generated allOf child it is identical to the
ancestry walk; for a consumer subclass that narrows attribute_map it
keeps the attributes to the ones the subclass accepts, as on master,
instead of passing its parent's fields to an initializer that rejects
them. The ancestry walk remains for openapi_types and openapi_nullable.

require 'set' is emitted only for generic models, the only ones whose
openapi_nullable uses Set; enum classes and oneOf/anyOf modules no
longer carry it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…table_attribute_map idiom

acceptable_openapi_types and acceptable_openapi_nullable merge along the
allOf ancestry the way acceptable_attribute_map already does, so
build_from_hash and to_hash lose their while loops and the require 'set'
block. Also drops the redundant map.key? guard, shortens the generated
comments and specs, and reverts the stray ruby-faraday Gemfile.lock bump.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@wiebren

wiebren commented Sep 23, 2026

Copy link
Copy Markdown
Contributor Author

Correction to my two comments above: in e49b322 the ancestry walks were replaced by parent-gated acceptable_openapi_types and acceptable_openapi_nullable class methods (the same idiom as acceptable_attribute_map). The nearest declaration still wins. The require 'set' addition is reverted, because nullability is evaluated lazily again as on master. The synthetic nullability-redeclare spec is replaced by a generator assertion on the merge line.

@cubic-dev-ai cubic-dev-ai Bot left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 222 files (changes from recent commits).

Note: This PR contains a large number of files. cubic selects up to 200 of the highest-priority eligible files for this review, so some files may not have been reviewed.
Tip: Review your code locally with the cubic CLI to iterate faster.

Re-trigger cubic

…elegate to it

An alias to an array (ArrayAlias < Array) has a parent, so the ancestry
helpers and the initializer called superclass methods Array does not
provide. Gate the delegation on parentModel, so every model has a base case.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@wing328

wing328 commented Sep 25, 2026

Copy link
Copy Markdown
Member

thanks for the fix. let's give it a try

@wing328
wing328 merged commit 9d0828b into OpenAPITools:master Sep 25, 2026
18 checks passed
@wing328 wing328 added Issue: Bug ruby Pull requests that update Ruby code labels Sep 25, 2026
@wing328 wing328 added this to the 7.26.0 milestone Sep 25, 2026
@wing328 wing328 added Client: Ruby and removed ruby Pull requests that update Ruby code labels Sep 25, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants